Skip to content

Kestrel Endpoints' "SslProtocols" settable via config (#22663) #22910

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 3 commits into from
Jun 19, 2020
Merged

Kestrel Endpoints' "SslProtocols" settable via config (#22663) #22910

merged 3 commits into from
Jun 19, 2020

Conversation

fstaffa
Copy link
Contributor

@fstaffa fstaffa commented Jun 13, 2020

Kestrel Endpoints' "SslProtocols" settable via config (#22663)

Priority of using SslProtocols is:

  1. Code default
  2. Configuration from EndpointDefaults section
  3. Code configuration for named endpoint
  4. Configuration from named endpoint section

@ghost ghost added the area-servers label Jun 13, 2020
@fstaffa
Copy link
Contributor Author

fstaffa commented Jun 13, 2020

I have tested the changes manually and they work ok. I have tried to write tests for the changes in KestrelConfigurationLoader but I didn't find a way to do so.
I can test the ConfigurationBackedListOptions (https://github.com/fstaffa/aspnetcore/blob/add-kestrel-ssl-protocols-in-config/src/Servers/Kestrel/Core/src/KestrelServerOptions.cs#L27 ) but that does not really test that the SslProtocols were correctly applied.
What I would like to test is results of
(https://github.com/fstaffa/aspnetcore/blob/add-kestrel-ssl-protocols-in-config/src/Servers/Kestrel/Core/src/KestrelConfigurationLoader.cs#L335 ) since this is the place which actually passes the SslProtocols configuration (as a part of httpsOptions) to the server. Do you knowhow to do that?

I also realized that changing EndpointDefaults in config does not cause configuration to reload. Is that expected or should I create an issue for that?

@fstaffa fstaffa changed the title manually tested version Kestrel Endpoints' "SslProtocols" settable via config (#22663) Jun 13, 2020
hassanhabib
hassanhabib reviewed Jun 14, 2020
View reviewed changes
@fstaffa fstaffa force-pushed the add-kestrel-ssl-protocols-in-config branch from f14119b to 0bc7f53 Compare June 14, 2020 10:18
@fstaffa fstaffa force-pushed the add-kestrel-ssl-protocols-in-config branch from 0bc7f53 to 4561eb9 Compare June 14, 2020 10:38
halter73
halter73 reviewed Jun 15, 2020
View reviewed changes
halter73
halter73 reviewed Jun 15, 2020
View reviewed changes
halter73
halter73 reviewed Jun 15, 2020
View reviewed changes
Tratcher
Tratcher approved these changes Jun 15, 2020
View reviewed changes
halter73
halter73 reviewed Jun 16, 2020
View reviewed changes
halter73
halter73 reviewed Jun 16, 2020
View reviewed changes
halter73
halter73 approved these changes Jun 16, 2020
View reviewed changes
Copy link
Member

@halter73 halter73 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like the nits addressed, but no big deal.

@halter73
Copy link
Member

Looks great. Thanks @fstaffa!

Tratcher
Tratcher approved these changes Jun 16, 2020
View reviewed changes
@fstaffa fstaffa force-pushed the add-kestrel-ssl-protocols-in-config branch from fe5f099 to a093450 Compare June 16, 2020 21:52
@fstaffa fstaffa force-pushed the add-kestrel-ssl-protocols-in-config branch from a093450 to 8545552 Compare June 16, 2020 21:58
@Tratcher Tratcher merged commit a77e68f into dotnet:master Jun 19, 2020
@Tratcher
Copy link
Member

Thanks

@Tratcher Tratcher added this to the 5.0.0-preview7 milestone Jun 19, 2020
@fstaffa fstaffa deleted the add-kestrel-ssl-protocols-in-config branch June 19, 2020 17:25
@halter73 halter73 mentioned this pull request Sep 21, 2020
Closed
@JamesNK JamesNK mentioned this pull request Sep 23, 2020
Closed
2 tasks
@amcasey amcasey added area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions and removed area-runtime labels Jun 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Tratcher Tratcher Tratcher approved these changes

@halter73 halter73 halter73 approved these changes

@jkotalik jkotalik Awaiting requested review from jkotalik

+1 more reviewer

@hassanhabib hassanhabib hassanhabib left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions

Projects

None yet

Milestone

5.0.0-preview7

Development

Successfully merging this pull request may close these issues.

5 participants

@fstaffa @halter73 @Tratcher @hassanhabib @amcasey